Repository navigation
fix(desktop): allow valid git characters in branch name sanitization - #1470
Conversation
The sanitizeSegment allowlist was too restrictive, only permitting
[a-z0-9\s-]. Expanded to also allow underscores, dots, plus signs,
and at signs — all valid per git-check-ref-format. Added guards for
git-forbidden patterns (.., @{, .lock suffix).
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughSanitization for branch-related strings was tightened and expanded: allowed characters now include underscores, dots, pluses, and at-signs; consecutive dots are collapsed; @{ sequences normalized; .lock suffix and leading/trailing dots/dashes removed. Tests and generateBranchFromTitle now use the updated sanitizeSegment behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/desktop/src/shared/utils/branch.ts`:
- Line 10: The leading-character strip regex in the replace call
(.replace(/^[-.]|[-.]+$/g, "")) only removes a single leading dot or dash;
update the pattern used in that replace to remove runs of leading mixed
dots/dashes symmetrically (e.g., change the left alternation to use a + like the
right side) so inputs like ".-hello" have all leading punctuation removed;
locate the replace invocation in branch.ts and replace the regex /^[-.]|[-.]+$/g
with a regex that uses + on the leading side (making it /^[-.]+|[-.]+$/g).
- Around line 6-8: The final .replace(/@\{/g, "@") call in the string
normalization chain is dead code because the earlier .replace(/[^a-z0-9._+@-]/g,
"") already strips the "{" character; remove the unreachable .replace(/@\{/g,
"@") from the replace chain in apps/desktop/src/shared/utils/branch.ts (the
normalization pipeline that applies .replace(/[^a-z0-9._+@-]/g, "") and
.replace(/\.{2,}/g, ".")). This keeps behavior identical and removes the
redundant operation; alternatively, if you prefer normalization of the original
input first, move the @\{→@ replacement to run before the allowlist replace, but
prefer simply deleting the dead .replace(/@\{/g, "@") call.
- Line 11: The current .replace(/\.lock$/g, "") only strips one trailing ".lock"
so names like "foo.lock.lock" remain invalid; update the .replace call in
branch.ts to use a pattern that matches one or more repeated ".lock" suffixes at
the end (i.e., a greedy/quantified pattern that removes all consecutive ".lock"
occurrences) so the sanitization fully strips any repeated ".lock" endings.
🧹 Nitpick comments (1)
apps/desktop/src/shared/utils/branch.test.ts (1)
21-47: Good coverage of the newly allowed characters and git-forbidden patterns.Consider adding edge-case tests for the issues noted in
branch.ts:// mixed leading chars test("removes multiple leading hyphens and dots", () => { expect(sanitizeSegment(".-hello")).toBe("hello"); }); // repeated .lock test("removes repeated .lock suffix", () => { expect(sanitizeSegment("foo.lock.lock")).toBe("foo"); });These would currently fail, confirming the bugs flagged in the implementation review.
| .replace(/[^a-z0-9._+@-]/g, "") | ||
| .replace(/\.{2,}/g, ".") | ||
| .replace(/@\{/g, "@") |
There was a problem hiding this comment.
@{ replacement on line 8 is dead code — { is already stripped by line 6.
The allowlist on line 6 ([^a-z0-9._+@-]) does not include {, so { is removed before the @{ replacement on line 8 ever runs. For example, "test@{0}" becomes "test@0" at line 6 already.
The test still passes because line 6 handles it, but line 8 is unreachable. Either remove line 8 or reorder lines 7-8 before line 6 so the @{ → @ normalization runs on the original input first.
Suggested fix: remove the dead line
.replace(/[^a-z0-9._+@-]/g, "")
.replace(/\.{2,}/g, ".")
- .replace(/@\{/g, "@")
.replace(/-+/g, "-")🤖 Prompt for AI Agents
In `@apps/desktop/src/shared/utils/branch.ts` around lines 6 - 8, The final
.replace(/@\{/g, "@") call in the string normalization chain is dead code
because the earlier .replace(/[^a-z0-9._+@-]/g, "") already strips the "{"
character; remove the unreachable .replace(/@\{/g, "@") from the replace chain
in apps/desktop/src/shared/utils/branch.ts (the normalization pipeline that
applies .replace(/[^a-z0-9._+@-]/g, "") and .replace(/\.{2,}/g, ".")). This
keeps behavior identical and removes the redundant operation; alternatively, if
you prefer normalization of the original input first, move the @\{→@ replacement
to run before the allowlist replace, but prefer simply deleting the dead
.replace(/@\{/g, "@") call.
| .replace(/@\{/g, "@") | ||
| .replace(/-+/g, "-") | ||
| .replace(/^-|-$/g, "") | ||
| .replace(/^[-.]|[-.]+$/g, "") |
There was a problem hiding this comment.
Leading-character strip only removes a single [-.], not a run of mixed chars.
/^[-.]|[-.]+$/g — the ^[-.] (no +) matches exactly one character. If the input after earlier steps has a sequence like .-hello, only the . is removed, leaving -hello. The trailing side correctly uses [-.]+$. The leading side should be symmetrical:
Suggested fix
- .replace(/^[-.]|[-.]+$/g, "")
+ .replace(/^[-.]+|[-.]+$/g, "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .replace(/^[-.]|[-.]+$/g, "") | |
| .replace(/^[-.]+|[-.]+$/g, "") |
🤖 Prompt for AI Agents
In `@apps/desktop/src/shared/utils/branch.ts` at line 10, The leading-character
strip regex in the replace call (.replace(/^[-.]|[-.]+$/g, "")) only removes a
single leading dot or dash; update the pattern used in that replace to remove
runs of leading mixed dots/dashes symmetrically (e.g., change the left
alternation to use a + like the right side) so inputs like ".-hello" have all
leading punctuation removed; locate the replace invocation in branch.ts and
replace the regex /^[-.]|[-.]+$/g with a regex that uses + on the leading side
(making it /^[-.]+|[-.]+$/g).
| .replace(/-+/g, "-") | ||
| .replace(/^-|-$/g, "") | ||
| .replace(/^[-.]|[-.]+$/g, "") | ||
| .replace(/\.lock$/g, "") |
There was a problem hiding this comment.
Repeated .lock suffix is not fully stripped.
A single .replace(/\.lock$/g, "") pass turns foo.lock.lock into foo.lock, which still violates git check-ref-format. Consider a greedy pattern:
Suggested fix
- .replace(/\.lock$/g, "")
+ .replace(/(\.lock)+$/g, "")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .replace(/\.lock$/g, "") | |
| .replace(/(\.lock)+$/g, "") |
🤖 Prompt for AI Agents
In `@apps/desktop/src/shared/utils/branch.ts` at line 11, The current
.replace(/\.lock$/g, "") only strips one trailing ".lock" so names like
"foo.lock.lock" remain invalid; update the .replace call in branch.ts to use a
pattern that matches one or more repeated ".lock" suffixes at the end (i.e., a
greedy/quantified pattern that removes all consecutive ".lock" occurrences) so
the sanitization fully strips any repeated ".lock" endings.
The project page had an inline generateBranchFromTitle with a duplicate of the old strict regex. Replace it with the shared sanitizeSegment utility so both flows use the same branch name logic.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Closes #1454
Closes #1455
Summary
sanitizeSegment) was too strict — only allowed[a-z0-9\s-], stripping valid git characters like underscores, dots, plus signs, and at signsgit-check-ref-formatrules while still guarding against forbidden patternsChanges
branch.ts: Changed allowlist from/[^a-z0-9\s-]/to/[^a-z0-9._+@-]/so underscores (_), dots (.), plus (+), and at signs (@) are preservedbranch.ts: Added guards for git-forbidden patterns: consecutive dots (..),@{sequences,.locksuffix, and leading/trailing dotsbranch.test.ts: Added tests for all newly allowed characters, git-forbidden pattern handling, and forbidden character stripping (~^:?*[\)Test Plan
bun test apps/desktop/src/shared/utils/branch.test.ts— 29 tests pass_,.,+,@and verify they are preserved~^:?*[\) are still strippedSummary by CodeRabbit
Bug Fixes
Tests